Conversation
Signed-off-by: Chojan Shang <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for field aliases in Pydantic models, specifically to map the field_meta attribute to the _meta JSON field for RPC serialization. The change enables proper serialization/deserialization of metadata fields using the JSON-RPC convention.
Key changes:
- Added
populate_by_name=Trueconfiguration to allow models to accept both field names and their aliases during instantiation - Updated serialization to use
by_alias=Trueto output JSON with aliased field names (_metainstead offield_meta) - Modified schema generation to automatically inject custom BaseModel configuration
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/acp/schema.py | Adds custom BaseModel class with populate_by_name=True configuration to support field aliases |
| src/acp/utils.py | Updates serialize_params to use by_alias=True for proper JSON field name mapping |
| src/acp/connection.py | Updates response serialization to use aliases and additional JSON-safe options |
| scripts/gen_schema.py | Adds schema generation logic to automatically inject custom BaseModel configuration |
| tests/test_utils.py | Adds test coverage for alias serialization behavior |
| examples/echo_agent.py | Demonstrates usage of field_meta attribute with the new alias support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+229
to
+251
| if not line.startswith("from pydantic import "): | ||
| continue | ||
| imports = [part.strip() for part in line[len("from pydantic import ") :].split(",")] | ||
| has_alias = any(part == "BaseModel as _BaseModel" for part in imports) | ||
| has_config = any(part == "ConfigDict" for part in imports) | ||
| new_imports = [] | ||
| for part in imports: | ||
| if part == "BaseModel": | ||
| new_imports.append("BaseModel as _BaseModel") | ||
| has_alias = True | ||
| else: | ||
| new_imports.append(part) | ||
| if not has_alias: | ||
| new_imports.append("BaseModel as _BaseModel") | ||
| if not has_config: | ||
| new_imports.append("ConfigDict") | ||
| lines[idx] = "from pydantic import " + ", ".join(new_imports) | ||
| insert_idx = idx + 1 | ||
| lines.insert(insert_idx, "") | ||
| lines.insert(insert_idx + 1, "class BaseModel(_BaseModel):") | ||
| lines.insert(insert_idx + 2, " model_config = ConfigDict(populate_by_name=True)") | ||
| lines.insert(insert_idx + 3, "") | ||
| break |
There was a problem hiding this comment.
The loop continues after finding the first from pydantic import line, but it should break after processing. This will cause issues if there are multiple pydantic import lines, as subsequent iterations will try to insert the BaseModel class definition multiple times at different positions.
Suggested change
| if not line.startswith("from pydantic import "): | |
| continue | |
| imports = [part.strip() for part in line[len("from pydantic import ") :].split(",")] | |
| has_alias = any(part == "BaseModel as _BaseModel" for part in imports) | |
| has_config = any(part == "ConfigDict" for part in imports) | |
| new_imports = [] | |
| for part in imports: | |
| if part == "BaseModel": | |
| new_imports.append("BaseModel as _BaseModel") | |
| has_alias = True | |
| else: | |
| new_imports.append(part) | |
| if not has_alias: | |
| new_imports.append("BaseModel as _BaseModel") | |
| if not has_config: | |
| new_imports.append("ConfigDict") | |
| lines[idx] = "from pydantic import " + ", ".join(new_imports) | |
| insert_idx = idx + 1 | |
| lines.insert(insert_idx, "") | |
| lines.insert(insert_idx + 1, "class BaseModel(_BaseModel):") | |
| lines.insert(insert_idx + 2, " model_config = ConfigDict(populate_by_name=True)") | |
| lines.insert(insert_idx + 3, "") | |
| break | |
| if line.startswith("from pydantic import "): | |
| imports = [part.strip() for part in line[len("from pydantic import ") :].split(",")] | |
| has_alias = any(part == "BaseModel as _BaseModel" for part in imports) | |
| has_config = any(part == "ConfigDict" for part in imports) | |
| new_imports = [] | |
| for part in imports: | |
| if part == "BaseModel": | |
| new_imports.append("BaseModel as _BaseModel") | |
| has_alias = True | |
| else: | |
| new_imports.append(part) | |
| if not has_alias: | |
| new_imports.append("BaseModel as _BaseModel") | |
| if not has_config: | |
| new_imports.append("ConfigDict") | |
| lines[idx] = "from pydantic import " + ", ".join(new_imports) | |
| insert_idx = idx + 1 | |
| lines.insert(insert_idx, "") | |
| lines.insert(insert_idx + 1, "class BaseModel(_BaseModel):") | |
| lines.insert(insert_idx + 2, " model_config = ConfigDict(populate_by_name=True)") | |
| lines.insert(insert_idx + 3, "") | |
| break |
Comment on lines
+232
to
+243
| has_alias = any(part == "BaseModel as _BaseModel" for part in imports) | ||
| has_config = any(part == "ConfigDict" for part in imports) | ||
| new_imports = [] | ||
| for part in imports: | ||
| if part == "BaseModel": | ||
| new_imports.append("BaseModel as _BaseModel") | ||
| has_alias = True | ||
| else: | ||
| new_imports.append(part) | ||
| if not has_alias: | ||
| new_imports.append("BaseModel as _BaseModel") | ||
| if not has_config: |
There was a problem hiding this comment.
The conditions if not has_alias and if not has_config will always be False when the loop at line 235-240 already found BaseModel and converted it to the alias. Since line 232-233 check for existing alias/config, and lines 236-238 ensure the alias is added when BaseModel is found, these additional checks are redundant. The logic should either rely on the existing checks (lines 232-233) or simplify this section.
Suggested change
| has_alias = any(part == "BaseModel as _BaseModel" for part in imports) | |
| has_config = any(part == "ConfigDict" for part in imports) | |
| new_imports = [] | |
| for part in imports: | |
| if part == "BaseModel": | |
| new_imports.append("BaseModel as _BaseModel") | |
| has_alias = True | |
| else: | |
| new_imports.append(part) | |
| if not has_alias: | |
| new_imports.append("BaseModel as _BaseModel") | |
| if not has_config: | |
| new_imports = [] | |
| for part in imports: | |
| if part == "BaseModel": | |
| new_imports.append("BaseModel as _BaseModel") | |
| else: | |
| new_imports.append(part) | |
| # Ensure "BaseModel as _BaseModel" and "ConfigDict" are present | |
| if "BaseModel as _BaseModel" not in new_imports: | |
| new_imports.append("BaseModel as _BaseModel") | |
| if "ConfigDict" not in new_imports: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix: #18